Add make_set function for DisjointSets #38692

thecaligarmo · 2024-09-22T01:53:09Z

This fixes #35599 by adding a make_set function to DisjointSet using OrbitPartitions. The documentation links to wikipedia.
From wikipedia, the method should be done in place and therefore there is no return

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

#35599

dcoudert

This is an interesting new functionality. However, why limiting to one extra element ?

dcoudert · 2024-09-22T08:13:25Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+    cdef int *new_parent, *new_rank, *new_mcr, *new_size
+
+    cdef int *int_array = <int *> sig_malloc( 4*(n+1) * sizeof(int) )
+    if int_array is NULL:


Do you really want to call free on NULL ?

Ah, sorry, I'll fix that.

dcoudert · 2024-09-22T08:15:29Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+        new_rank = int_array + (n + 1)
+        new_mcr = int_array + (2*n + 2)
+        new_size = int_array + (3 * n + 3)
+        for i from 0 <= i < n:


for i in range(n)

Will that work for c? If you look at all the other for loops in the .pxd file they all have the same format:
for i from 0 <= i < n:

yes, it will work.

K, I've gone ahead and changed it.

For future: is there a reason why all the other ones use for i from 0 <= i < n? Because every single other for loop in the file uses this syntax rather than the for i in range syntax.

These methods have been written long time ago and Cython has evolved (improved) since. We have not update all the code yet, it's too big.

dcoudert · 2024-09-22T08:23:02Z

src/sage/sets/disjoint_set.pyx

@@ -834,6 +854,43 @@ cdef class DisjointSet_of_hashables(DisjointSet_class):
        cdef int j = <int> self._el_to_int[f]
        OP_join(self._nodes, i, j)

+    def make_set(self, new_elt = None):
+        r"""
+        Return a new disjoint set with an additional item.


You are not returning a new set, you are adding one new element to the set

dcoudert · 2024-09-22T08:24:12Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+        new_size = int_array + (3 * n + 3)
+        for i from 0 <= i < n:
+            new_parent[i] = OP.parent[i]
+            new_rank[i] = OP.rank[i]


why not using low level memcpy method ?

I'm not perfect with c, but if I understand correctly, memcpy will also copy the amount of memory allocated for the object. Since we are working with arrays, that would mean that the new copy would not have enough memory allocated to it since we need to add an additional entry into the array.

The first option is to make 4 different mallocs for parent, rank, etc. when first creating the data structure.
Then, in make_set, you can use realloc to extend the size of the arrays. Realloc should copy the data for you and so you only have to allocate the last value. In case of error, one of these arrays will get a NULL pointer (unless I'm mistaken). Last, when deallocating, you have to do 4 free.

The other option is to use memcpy instead of the for loop to put the good values in new_.... It should be a call like memcpy(new_parent, OP.parent, n). It remains to assign the last cell of new_parent.

Ok, I'm not as comfortable with C, so I'll see if I can try and use these methods.

thecaligarmo · 2024-09-22T12:08:27Z

This is an interesting new functionality. However, why limiting to one extra element ?

From what I understand from reading wikipedia, DisjointSet is a set of singleton sets where you can put sets together (union) and find where certain elements are located (find). make_set then allows you to add a new singleton set to your DisjointSet. From what I understand, you wouldn't want to add a set of multiple elements (as this defeats the point of DisjointSet) and if you want to add multiple singleton sets, you can call make_set multiple times. In essence, I was trying to use wiki's implementation idea for make_set.

dcoudert · 2024-09-22T13:06:23Z

This is an interesting new functionality. However, why limiting to one extra element ?

From what I understand from reading wikipedia, DisjointSet is a set of singleton sets where you can put sets together (union) and find where certain elements are located (find). make_set then allows you to add a new singleton set to your DisjointSet. From what I understand, you wouldn't want to add a set of multiple elements (as this defeats the point of DisjointSet) and if you want to add multiple singleton sets, you can call make_set multiple times. In essence, I was trying to use wiki's implementation idea for make_set.

I agree that make_set is designed to add a new singleton set. But we could also add several new singleton sets at once to avoid multiple calls to make_set.

thecaligarmo · 2024-09-22T14:07:17Z

This is an interesting new functionality. However, why limiting to one extra element ?

From what I understand from reading wikipedia, DisjointSet is a set of singleton sets where you can put sets together (union) and find where certain elements are located (find). make_set then allows you to add a new singleton set to your DisjointSet. From what I understand, you wouldn't want to add a set of multiple elements (as this defeats the point of DisjointSet) and if you want to add multiple singleton sets, you can call make_set multiple times. In essence, I was trying to use wiki's implementation idea for make_set.

I agree that make_set is designed to add a new singleton set. But we could also add several new singleton sets at once to avoid multiple calls to make_set.

Yes, but it becomes ambiguous once you do that. For example, if I were to do: d.make_set([1, 2]) is this adding the set [1, 2] or is it adding 2 sets 1 and 2? The function make_set makes it sound like I would be adding only 1 set, not multiple sets, so my initial assumption would be that make_set([1, 2]) would add a exactly 1 new set with entry [1, 2].

We could still make a function that does allow making multiple sets (say make_sets) which could then go through and do all the set creations at once (change OP_make_set to allow an optional second parameter). Or if you want to keep it as make_set, we can add an additional parameter called multiple that's a boolean that people can specify to show that they are adding multiple things instead of just one? so make_set([1, 2]) would add 1 set [1, 2] and make_set([1, 2], multiple=True) would create two sets 1 and 2.

dcoudert · 2024-09-22T14:11:20Z

Let's keep this for a future PR.

thecaligarmo · 2024-09-22T14:21:04Z

Sounds good.

I also tried to use the memcpy approach you had mentioned above. I ran the tests on my end and they seemed to work, but if there's an error, let me know. I wasn't sure if the 3rd parameter needed changing for the other 3 variables, but due to the previous memory allocation, I'm assuming not.

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

Fix memcpy allocation

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

src/sage/sets/disjoint_set.pyx

dcoudert · 2024-09-23T08:54:58Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+    cdef int *new_parent, *new_rank, *new_mcr, *new_size
+
+    cdef int *int_array = <int *> sig_malloc( 4*(n+1) * sizeof(int) )
+    if int_array is not NULL:


What if int_array is NULL ? you should certainly raise an error.

dcoudert · 2024-09-23T08:55:28Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+        new_size[n] = 1
+
+        OP.parent = new_parent
+        OP.rank   = new_rank


please avoid the extra spaces before =. This is not compatible with Python coding style.

dcoudert · 2024-09-23T08:56:43Z

src/sage/sets/disjoint_set.pyx

+
+        INPUT:
+
+        - ``new_elt`` -- (optional) element to add. If `None`, then an integer


for the second line, you must add 2 extra spaces, like:

- ``new_elt`` -- (optional) element to add. If `None`, then an integer is added.

this currently breaks building the pdf documentation (reported by CI).

dcoudert · 2024-09-23T08:57:42Z

src/sage/sets/disjoint_set.pyx

@@ -834,6 +854,43 @@ cdef class DisjointSet_of_hashables(DisjointSet_class):
        cdef int j = <int> self._el_to_int[f]
        OP_join(self._nodes, i, j)

+    def make_set(self, new_elt = None):


don't put spaces around = here. It should be def make_set(self, new_elt=None):

dcoudert · 2024-09-23T14:54:58Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+    cdef int *int_array = <int *> sig_malloc(4*(n+1) * sizeof(int))
+    if int_array is NULL:
+        raise MemoryError("MemoryError allocating int_array in make_set method")
+    else:


actually, you can remove the else and reduce the indentation of the code. Indeed, the code will be executed only if int_array is not NULL.

That's a good point. I'll switch it over.

github-actions · 2024-09-23T15:14:26Z

Documentation preview for this PR (built with commit 598ccfd; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

dcoudert · 2024-09-23T17:38:39Z

src/sage/groups/perm_gps/partn_ref/data_structures.pxd

+    new_mcr[n] = n
+    new_size[n] = 1
+
+    OP.parent = new_parent


before this statement, you must release the memory used by the previous arrays, so sig_free(OP.parent).

Done. I'm not so good with memory allocation stuff, but I think we only need sig_free(OP.parent) correct? I tried adding the other ones and it started throwing errors, so I'm thinking that OP.parent holds the entire memory for all 4 variables so freeing up the memory from that element does it for all 4. If I did that wrong, just let me know.

dcoudert

LGTM.

This fixes sagemath#35599 by adding a `make_set` function to `DisjointSet` using `OrbitPartitions`. The documentation links to wikipedia. From wikipedia, the method should be done in place and therefore there is no return ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    sagemath#35599 URL: sagemath#38692 Reported by: Aram Dermenjian Reviewer(s): Aram Dermenjian, David Coudert

Add make_set function for DisjointSets

21417a4

github-actions bot added the s: needs review label Sep 22, 2024

Allow for adding elt to hashable version without specifying

d173d35

dcoudert suggested changes Sep 22, 2024

View reviewed changes

Fix minor things from comments

5080482

Change for loop

76e2b1c

Switch to memcpy

c0b9fd9

Merge branch 'develop' into make_set-for-DisjointSet

baf6539

dcoudert suggested changes Sep 22, 2024

View reviewed changes

src/sage/groups/perm_gps/partn_ref/data_structures.pxd Outdated Show resolved Hide resolved

Update data_structures.pxd

2c26072

Fix memcpy allocation

dcoudert reviewed Sep 22, 2024

View reviewed changes

src/sage/groups/perm_gps/partn_ref/data_structures.pxd Outdated Show resolved Hide resolved

src/sage/sets/disjoint_set.pyx Outdated Show resolved Hide resolved

Fix python

b9cd6d2

dcoudert reviewed Sep 23, 2024

View reviewed changes

Code style fixes

1d0df0e

dcoudert reviewed Sep 23, 2024

View reviewed changes

Fix indenting

5ff7cac

dcoudert reviewed Sep 23, 2024

View reviewed changes

Free up memory from OP.parent

598ccfd

dcoudert approved these changes Sep 24, 2024

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Sep 24, 2024

vbraun merged commit 40cb8c2 into sagemath:develop Sep 29, 2024
21 checks passed

github-actions bot removed the s: positive review label Sep 29, 2024

dcoudert mentioned this pull request Nov 7, 2024

use sig_realloc in method OP_make_set #38936

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add make_set function for DisjointSets #38692

Add make_set function for DisjointSets #38692

thecaligarmo commented Sep 22, 2024

dcoudert left a comment

dcoudert Sep 22, 2024

thecaligarmo Sep 22, 2024

dcoudert Sep 22, 2024

thecaligarmo Sep 22, 2024

dcoudert Sep 22, 2024

thecaligarmo Sep 22, 2024

dcoudert Sep 22, 2024

dcoudert Sep 22, 2024

dcoudert Sep 22, 2024

thecaligarmo Sep 22, 2024

dcoudert Sep 22, 2024

thecaligarmo Sep 22, 2024

thecaligarmo commented Sep 22, 2024

dcoudert commented Sep 22, 2024

thecaligarmo commented Sep 22, 2024

dcoudert commented Sep 22, 2024

thecaligarmo commented Sep 22, 2024

dcoudert Sep 23, 2024

dcoudert Sep 23, 2024

dcoudert Sep 23, 2024

dcoudert Sep 23, 2024

dcoudert Sep 23, 2024

thecaligarmo Sep 23, 2024

github-actions bot commented Sep 23, 2024 •

edited

Loading

dcoudert Sep 23, 2024

thecaligarmo Sep 24, 2024

dcoudert left a comment


		INPUT:

		- ``new_elt`` -- (optional) element to add. If `None`, then an integer

Add make_set function for DisjointSets #38692

Add make_set function for DisjointSets #38692

Conversation

thecaligarmo commented Sep 22, 2024

📝 Checklist

⌛ Dependencies

dcoudert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecaligarmo commented Sep 22, 2024

dcoudert commented Sep 22, 2024

thecaligarmo commented Sep 22, 2024

dcoudert commented Sep 22, 2024

thecaligarmo commented Sep 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoudert left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2024 •

edited

Loading